tech debt: update ledger dependencies #346

0xMillz · 2021-10-14T23:19:31Z

Also removes u2f which Ledger has deprecated

vercel · 2021-10-14T23:19:33Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shapeshift/hdwallet/5vJwgoA5rtMuU1bCqexW28VGfk9P
✅ Preview: https://hdwallet-git-updateledgerdependencies-shapeshift.vercel.app

[Deployment for 599f164 failed]

cjthompson · 2021-10-15T01:02:15Z

examples/sandbox/package.json

@@ -9,16 +9,19 @@
    "dev": "yarn parcel index.html",
    "clean": "rm -rf dist .cache"
  },
+  "engines": {
+    "node": "14.x"


I don't think this is accurate. You're 100% certain this doesn't run on older versions of node? I'm running node 12.x and it compiles fine for me.

cjthompson · 2021-10-15T01:06:47Z

packages/hdwallet-core/package.json

@@ -13,6 +13,9 @@
    "clean": "rm -rf dist tsconfig.tsbuildinfo",
    "prepublishOnly": "yarn clean && yarn build"
  },
+  "engines": {
+    "node": "14.x"


I know this is wrong because we use hdwallet in axiom which runs node 12. "engines" will prevent use of any other version of node. If you absolutely want to add "engines" then use ">= 12" or whatever the minimum version of node is that is compatible.

cjthompson

remove or change the engines property in package.json.

mrnerdhair · 2021-10-14T23:24:36Z

package.json

+  "engines": {
+    "node": "14.x"
+  },


We removed engine restrictions on purpose several versions ago. If a dependency requires at least node 14, then it's own engines block should be set; if it isn't, we should set it on the affected hdwallet-* package, but it shouldn't be all-pervasive, and should at least specify >=14 instead of 14.x.

packages/hdwallet-ledger-webusb/package.json

mrnerdhair · 2021-10-14T23:31:57Z

packages/hdwallet-ledger-webusb/src/transport.ts

@@ -10,19 +10,20 @@ import * as ledger from "@shapeshiftoss/hdwallet-ledger";

 const RECORD_CONFORMANCE_MOCKS = false;

-function translateCoin(coin: string): { new (transport: Transport): Record<string, (...args: any[]) => unknown> } {
-  return core.mustBeDefined(({
+function translateCoin(coin: string): any | unknown {


any | unknown is just any, which probably isn't what's intended, and if it is it probably shouldn't be.

Suggested change

function translateCoin(coin: string): any | unknown {

function translateCoin(coin: string): äny! äny! Cthulhu fhtagn! {

packages/hdwallet-ledger/package.json

mrnerdhair · 2021-10-14T23:59:22Z

packages/hdwallet-ledger/src/utils.ts

+  return {
    [core.BTCInputScriptType.SpendAddress]: "legacy",
    [core.BTCInputScriptType.CashAddr]: "legacy",
    [core.BTCInputScriptType.SpendWitness]: "bech32",
    [core.BTCInputScriptType.SpendP2SHWitness]: "p2sh",
-  } as Partial<Record<core.BTCInputScriptType, string>>)[scriptType]);
+  }[scriptType];


This leaves no runtime check that all core.BTCInputScriptType options are accounted for. A switch statement without a default case would instead provide a static guarantee of full coverage -- though it would still need a default throw for forwards-compatibility. I'd argue it's too verbose, and a simple map and throw-if-undefined (a la core.mustBeDefined) is preferable.

Suggested change

return {

[core.BTCInputScriptType.SpendAddress]: "legacy",

[core.BTCInputScriptType.CashAddr]: "legacy",

[core.BTCInputScriptType.SpendWitness]: "bech32",

[core.BTCInputScriptType.SpendP2SHWitness]: "p2sh",

} as Partial<Record<core.BTCInputScriptType, string>>)[scriptType]);

}[scriptType];

switch (scriptType) {

case core.BTCInputScriptType.SpendAddress:

return "legacy";

case core.BTCInputScriptType.CashAddr:

return "legacy";

case core.BTCInputScriptType.SpendWitness:

return "bech32";

case core.BTCInputScriptType.SpendP2SHWitness:

return "p2sh";

}

mrnerdhair · 2021-10-15T00:29:07Z

packages/hdwallet-ledger/src/bitcoin.ts

+    const keySet = msg.inputs[i].addressNList
+      .map((num) => (num >= core.HARDENED ? `${num - core.HARDENED}'` : num))
+      .join("/")


Suggested change

const keySet = msg.inputs[i].addressNList

.map((num) => (num >= core.HARDENED ? `${num - core.HARDENED}'` : num))

.join("/")

const keySet = core.addressNListToBIP32(msg.inputs[i].addressNList).replace(/^m\//, "")

This is what I originally coded but either the hot-reload failed to pick up my changes or I screwed up the regex and it wasn't working. I'll see if I can pull it off this time.

mrnerdhair · 2021-10-15T00:48:39Z

packages/hdwallet-core/src/utils.ts

@@ -58,7 +58,7 @@ export function arrayify(value: string): Uint8Array {
  return new Uint8Array(result);
 }

-const HARDENED = 0x80000000;
+export const HARDENED = 0x80000000;


IMO this would need a better name for export at this level; also it doesn't need to be exported if if you use core.addressNListToBIP32 as shown below.

Suggested change

export const HARDENED = 0x80000000;

const HARDENED = 0x80000000;

mrnerdhair · 2021-10-15T01:12:58Z

packages/hdwallet-ledger/src/ledger.ts


-    const appName = _.get(networksUtil[core.mustBeDefined(core.slip44ByCoin(coin))], "appName");
-    if (!appName) {
-      throw new Error(`Unable to find associated app name for coin: ${coin}`);
-    }
-
    const res = await this.transport.call(null, "getAppAndVersion");
    handleError(res, this.transport);

    const {
      payload: { name: currentApp },
    } = res;
-    if (currentApp !== appName) {
-      throw new core.WrongApp("Ledger", appName);
+    if (currentApp !== coin) {
+      throw new core.WrongApp("Ledger", coin);


Ledger app names do not equal hdwallet-core's Coin names. Note for example BitcoinCash vs Bitcoin Cash and Digibyte vs DigiByte. I don't think you can avoid a lookup of some kind here.

mrnerdhair · 2021-10-15T03:02:17Z

packages/hdwallet-ledger-webusb/src/transport.ts

-  return core.mustBeDefined(({
+function translateMethod(method: string): (transport: Transport, ...args: any[]) => any {
+  return ({
+    decorateAppAPIMethods: TransportWebUSB.decorateAppAPIMethods,


I don't think this is a static method -- does this work at runtime?

mrnerdhair · 2021-10-15T04:17:25Z

packages/hdwallet-ledger/src/bitcoin.ts

@@ -14,7 +14,7 @@ import {
  handleError,
 } from "./utils";

-const supportedCoins = ["Testnet", "Bitcoin", "BitcoinCash", "Litecoin", "Dash", "DigiByte", "Dogecoin"];
+export const supportedCoins = ["Testnet", "Bitcoin", "BitcoinCash", "Litecoin", "Dash", "DigiByte", "Dogecoin"];


This export is not needed since btcSupportsCoin is already available.

mrnerdhair · 2021-10-16T02:49:49Z

(whoops, wrong branch!)

mrnerdhair · 2021-10-16T03:35:02Z

@millsmcilroy I did some cleanup on this and added the TS magic needed to pull in the types from the ledger packages. It needed a couple of supporting PRs (#349, #350) to enable strict mode first; since we use null to switch to a specific set of methods in call(null, "foo"), strictNullChecks needs to be turned on so that null isn't treated as a member of every type.

I opened #351 separately to avoid clobbering your branch history with the extra strict-mode stuff, but if you like it IMO it'd make more sense to force-push it to this branch and use this PR instead of that one.

0xMillz · 2021-10-18T21:51:26Z

Closing in favor of #351

tech debt: update ledger dependencies

319c09d

mrnerdhair self-requested a review October 14, 2021 23:48

cjthompson reviewed Oct 15, 2021

View reviewed changes

cjthompson suggested changes Oct 15, 2021

View reviewed changes

mrnerdhair suggested changes Oct 15, 2021

View reviewed changes

mrnerdhair mentioned this pull request Oct 16, 2021

Update Ledger Deps (2) #351

Merged

mrnerdhair force-pushed the update_ledger_dependencies branch from 319c09d to 599f164 Compare October 16, 2021 02:41

vercel bot had a problem deploying to Preview October 16, 2021 02:41 Failure

mrnerdhair force-pushed the update_ledger_dependencies branch from 599f164 to 319c09d Compare October 16, 2021 02:49

0xMillz closed this Oct 18, 2021

0xMillz deleted the update_ledger_dependencies branch October 18, 2021 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tech debt: update ledger dependencies #346

tech debt: update ledger dependencies #346

0xMillz commented Oct 14, 2021 •

edited

Loading

vercel bot commented Oct 14, 2021 •

edited

Loading

cjthompson Oct 15, 2021

cjthompson Oct 15, 2021

cjthompson left a comment

mrnerdhair Oct 14, 2021

mrnerdhair Oct 14, 2021

mrnerdhair Oct 14, 2021

mrnerdhair Oct 15, 2021

0xMillz Oct 15, 2021

mrnerdhair Oct 15, 2021

mrnerdhair Oct 15, 2021

mrnerdhair Oct 15, 2021

mrnerdhair Oct 15, 2021

mrnerdhair commented Oct 16, 2021

mrnerdhair commented Oct 16, 2021

0xMillz commented Oct 18, 2021

	function translateCoin(coin: string): any \| unknown {
	function translateCoin(coin: string): äny! äny! Cthulhu fhtagn! {

-  return {
-    [core.BTCInputScriptType.SpendAddress]: "legacy",
-    [core.BTCInputScriptType.CashAddr]: "legacy",
-    [core.BTCInputScriptType.SpendWitness]: "bech32",
-    [core.BTCInputScriptType.SpendP2SHWitness]: "p2sh",
-  } as Partial<Record<core.BTCInputScriptType, string>>)[scriptType]);
-  }[scriptType];
+    switch (scriptType) {
+      case core.BTCInputScriptType.SpendAddress:
+        return "legacy";
+      case core.BTCInputScriptType.CashAddr:
+        return "legacy";
+      case core.BTCInputScriptType.SpendWitness:
+        return "bech32";
+      case core.BTCInputScriptType.SpendP2SHWitness:
+        return "p2sh";
+    }

	export const HARDENED = 0x80000000;
	const HARDENED = 0x80000000;

tech debt: update ledger dependencies #346

tech debt: update ledger dependencies #346

Conversation

0xMillz commented Oct 14, 2021 • edited Loading

vercel bot commented Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjthompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnerdhair commented Oct 16, 2021

mrnerdhair commented Oct 16, 2021

0xMillz commented Oct 18, 2021

0xMillz commented Oct 14, 2021 •

edited

Loading

vercel bot commented Oct 14, 2021 •

edited

Loading